Conversation
lumirlumir
added
the
accepted
✅ Deploy Preview for eslint-code-explorer ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@lumirlumir, I wanted to share some constructive feedback on your PRs:
|
There was a problem hiding this comment.
Pull request overview
Adds support in Code Explorer for the new math parsing option in the Markdown language (introduced in @eslint/markdown v8), including a UI toggle and an e2e test update.
Changes:
- Add a new
markdownMathboolean option (type + default) and expose it in the Markdown options panel. - Pass
maththrough to@eslint/markdown’s parser vialanguageOptions. - Bump
@eslint/markdownto^8.0.1and extend the e2e options-panel test to assert the new switch is visible.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/const.ts | Adds markdownMath to the default Markdown options. |
| src/hooks/use-explorer.ts | Extends MarkdownOptions to include markdownMath. |
| src/hooks/use-ast.ts | Wires markdownMath into the markdown parser languageOptions. |
| src/components/options.tsx | Adds a new “Math” switch to the Markdown options UI. |
| package.json | Updates @eslint/markdown dependency to v8. |
| package-lock.json | Lockfile updates for @eslint/markdown@8 and its transitive deps/engines. |
| e2e-tests/options.test.ts | Adds an assertion that the “Math” switch is visible for Markdown. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <LabeledSwitch | ||
| id="markdownMath" | ||
| label="Math" | ||
| checked={markdownMath} |
There was a problem hiding this comment.
checked={markdownMath} assumes the value is always a boolean, but if a user has older persisted state (before markdownMath existed) it can be undefined and cause controlled/uncontrolled issues. Either ensure rehydration/migration fills markdownMath, or defensively coalesce here (e.g. default to false).
| checked={markdownMath} | |
| checked={markdownMath ?? false} |
| markdownFrontmatter === "off" | ||
| ? false | ||
| : markdownFrontmatter, | ||
| math: markdownMath, |
There was a problem hiding this comment.
If markdownMath is missing from rehydrated persisted state, math: markdownMath can be undefined. Consider defaulting this to false here (and/or adding a persist migration) so the parser option is always a boolean.
| math: markdownMath, | |
| math: markdownMath ?? false, |
| markdownFrontmatter: MarkdownFrontmatter; | ||
| markdownMath: boolean; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Adding a new field to MarkdownOptions can break existing persisted state: zustand persist merges state shallowly, so older markdownOptions objects will overwrite defaultMarkdownOptions and may omit markdownMath, yielding undefined at runtime. Consider adding a persist version + migrate/custom merge, or patching rehydrated markdownOptions to include markdownMath: false when missing.
| type PersistedMarkdownOptions = Omit<MarkdownOptions, "markdownMath"> & { | |
| markdownMath?: boolean; | |
| }; | |
| export const normalizeMarkdownOptions = ( | |
| markdownOptions?: PersistedMarkdownOptions, | |
| ): MarkdownOptions => ({ | |
| ...defaultMarkdownOptions, | |
| ...markdownOptions, | |
| markdownMath: markdownOptions?.markdownMath ?? false, | |
| }); |
There was a problem hiding this comment.
Okay, I'll dig into this problem a bit more in the near future.
Prerequisites checklist
What is the purpose of this pull request?
This PR updates Code Explorer to support the Math parsing option for the Markdown language, introduced in eslint/markdown#617 and released as part of
@eslint/markdownv8.I've also updated
e2e-tests/options.test.tsto include tests for the newly added Math option.What changes did you make? (Give an overview)
Related Issues
Ref: eslint/markdown#331
Is there anything you'd like reviewers to focus on?
Actually, adding AST node interaction and expansion tests would make this feature more robust, but there are currently no tests for those cases. I’d like to continue the work on #398, since it requires many of the changes.